Refactor behavior spec#14
Conversation
startHydraNode is deliberately not parameterized to illustrate the benefit of using io-sim instead of IO in this test suite.
ch1bo
force-pushed
the
refactor-behavior-spec
branch
from
589ccbd to
d99cd36
Compare
Also, 'expectationFailure' got replaced by 'error' (for now). Maybe use 'MonadThrow' instead?
ch1bo
force-pushed
the
refactor-behavior-spec
branch
from
d99cd36 to
1dd2c99
Compare
ch1bo
force-pushed
the
refactor-behavior-spec
branch
from
1dd2c99 to
2df2eba
Compare
This also required more lifted variants of hspec-expectations
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Some comments mostly about the introduction of Test.Util: I like it and would go farther and use it as a Test.Prelude exposing standard stuff from HSpec.
| import Network.TypedProtocol.Channel (createConnectedChannels) | ||
| import Network.TypedProtocol.Driver.Simple (runPeer) | ||
| import Test.Hspec.Core.Spec | ||
| import Test.Util (shouldBe, shouldRunInSim) |
There was a problem hiding this comment.
Why not using Test.Hspec which is the more general public interface of Hspec?
There was a problem hiding this comment.
Perhaps we could reexport Test.Hspec from Test.Util in order to start building a "Test Prelude" unifying how we write and run tests in Hydra?
There was a problem hiding this comment.
This is a relict from where I tried to shuffle things a bit.. I like the idea of a Test.Prelude though!
| import GHC.Stack (SrcLoc, callStack) | ||
| import Test.HUnit.Lang (FailureReason (ExpectedButGot, Reason), HUnitFailure (HUnitFailure)) | ||
|
|
||
| failure :: (HasCallStack, MonadThrow m) => String -> m a |
There was a problem hiding this comment.
Are we settling for tuple-style or arguments-style constraints? Seems like we are using both in the code base, could be something to put in our coding conventions.
There was a problem hiding this comment.
If we have to settle on one style, I'd cast a vote for the tuple-style 😬
| (_, loc) : _ -> Just loc | ||
| _ -> Nothing | ||
|
|
||
| failAfter :: (HasCallStack, MonadTimer m, MonadThrow m) => DiffTime -> m () -> m () |
| import System.Timeout (timeout) | ||
| import Test.Hspec (Spec, describe, it, shouldReturn) | ||
| import Test.Hspec.Core.Spec (Spec, describe, it) | ||
| import Test.Util (shouldReturn) |
There was a problem hiding this comment.
Same remarks than for FireForgetSpec: We should reexport Test.Hspec from Test.Util and simplify imports for testing purpose
|
|
||
| it "accepts a tx after the head was opened between two nodes" $ do | ||
| it "is Ready when started" $ | ||
| shouldRunInSim $ do |
There was a problem hiding this comment.
Given that all tests use shouldRunInSim perhaps we could have a specialised it combinator for that?
There was a problem hiding this comment.
Thought about that quickly, but then put it off as being about the same overhead in typing/reading. I have no preference..what would you call that it variation?
This is to avoid resource leakage. However there is still a "SloppyShutdown" reported by io-sim's runSimStrictShutdown for some cases. Furthermore, the HydraProcess handle was renamed to 'TestHydraNode' as I did not manage to get rid of it. Specifically it is required to 'waitForResponse'.
2 participants
Run
BehaviorSpec(andFireForgetSpec) inIOSimThis is primarily to deal with longer contestationPeriods.
Next steps:
runSimStrictShutdownand ensure proper thread clean-up inBehaviorSpec-> most threads are cleaned up, but not allHydraProcess(and useHydraNodeinstead) -> could not get rid of it fully